fix: fixed cumulativeGasUsed calculations in eth_getBlockReceipts and eth_getTransactionReceipt (#4921)#4936
Conversation
… eth_getTransactionReceipt (hiero-ledger#4921) Signed-off-by: Bartosz Solka <bartosz.solka@blockydevs.com>
jasuwienas
left a comment
jasuwienas
left a comment
There was a problem hiding this comment.
Looks great overall, but I have a few questions.
| for (const item of items) { | ||
| const { transactionIndex, logsPerTx, crPerTx } = item; | ||
|
|
||
| const gasUsed = crPerTx.length && crPerTx[0].gas_used != null ? crPerTx[0].gas_used : 0; |
There was a problem hiding this comment.
| const gasUsed = crPerTx.length && crPerTx[0].gas_used != null ? crPerTx[0].gas_used : 0; | |
| const gasUsed = crPerTx[0]?.gas_used ?? 0; |
|
|
||
| const gasUsed = crPerTx.length && crPerTx[0].gas_used != null ? crPerTx[0].gas_used : 0; | ||
| cumulativeGas += gasUsed; | ||
| const transactionIndexHex = intToHex(transactionIndex); |
There was a problem hiding this comment.
We still might have -1 here...
There was a problem hiding this comment.
I will use 0 as a fallback value. Do you see any risks in doing that?
| const receiptPromises = contractResults.map(async (contractResult) => { | ||
| // Ensure contract results are processed in transaction_index (block) order | ||
| const sortedContractResults = [...contractResults].sort((a, b) => { | ||
| const aIdx = a.transaction_index ?? Number.MAX_SAFE_INTEGER; |
There was a problem hiding this comment.
What should we sort by eventually? In previous method we used:
crPerTx[0].transaction_index
but when it was missing we checked:
logsPerTx[0].transactionIndex
now we are taking only cr, right?
Since we are fetching them all with a single request woulnd't it be posisble to fget them fetched from mirrorndoe right away, instead of sorting them on our own?
Aren;'t they, by any chance already sorted?
There was a problem hiding this comment.
I think in this case, we should first create all receipts, both regular and synthetic, and then do another loop to calculate cumulativeGasUsed for each.
Synthetic receipts have 0 gasUsed, but it doesn't imply they should have 0 as cumulativeGasUsed. Please let me know what you think about this approach.
There was a problem hiding this comment.
Ok, I get it now, synthetics dont ever influnce this value since they always use 0 gas.In this case I don't think we should take them into consideration here.
BUT we need to set this cumulativeGasPrice for the synthetic receipts generated here:
https://github.com/hiero-ledger/hiero-json-rpc-relay/pull/4936/changes/BASE..586dec6a6ba4dd355f07abae9d2425755e928503#diff-b8e1972336c5bbc108e68c01afa6bf2999c059bf3ea9ee67642d8a906f57a45eR444
as well. Which we aren't doing now, right?
| const logsPerTx: Log[] = logs.filter((log) => log.transactionHash === txHash); | ||
| const crPerTx: any[] = contractResults.filter((cr) => cr.hash === txHash); |
There was a problem hiding this comment.
nit: Don't we really know what is the type of crPerTx?
There was a problem hiding this comment.
I think we should be able to type it. Maybe we can create another issue to fix it everywhere in the codebase?
There was a problem hiding this comment.
Sure, it’s not that important, it just crossed my mind while I was checking the PR. We can address it in a separate issue, we already have this problem in the code.
| const params: IContractResultsParams = { | ||
| blockNumber: receiptResponse.block_number, | ||
| }; | ||
|
|
||
| const blockContractResults = await this.mirrorNodeClient.getContractResults(requestDetails, params); |
There was a problem hiding this comment.
OK, I really don’t like the idea of fetching heavy block data for every transaction just to retrieve this single value :/ . But I don't know what to do about that, I really think this should be just calcualted in the mirrornode.
q:
Is there any correlation between the transaction index and the timestamp? Since this is the Hashgraph overall, consensus timestamps should matter for ordering, right? Maybe it applies to the on-block orders and we can fetch only the transactions with a timestamp lower than or equal to the one we’re querying for? (or would there be a problem with internal transactions for examlple?)
Also, we’re ignoring the fallback tx index from logs and only using the one from contract results. Wont that create differences for a single transaction when comparing it across these different endpoints?
There was a problem hiding this comment.
https://docs.hedera.com/api-reference/contracts/list-contract-results-from-all-contracts-on-the-network#parameter-transaction-index - there is an option to even query by transaction index x >= 0. A thing to discuss is, if we want to ask mirror node team to calculate this value on their side, or we do it ourselves and optimize what we can
There was a problem hiding this comment.
Honestly, I didn’t anticipate all of these implications when we were preparing the initial issue. What I do know is that fetching the block can be a very heavy operation (and I think it happens far less frequently than fetching a transaction receipt).
Including this could have significant performance implications with potentially limited benefit. I think we should all consider the options:
- Ignore this value for this particular endpoint - worst solution imo, since it would make the same transaction look different depending on where it’s queried.
- Accept the performance impact (investigate how significant it actually is) and proceed with the full implementation. We should also try to filter the results as much as possible. If filtering by transaction index works (assuming it’s the same transaction.index we’re referring to: I’m not entirely sure, as it sometimes appears to come from logs) or timestamps, we should do it deffinitely.
- (Keep the current “fake” value for cumulativeGasUsed - but clearly document that behavior and do this operation on the MN side.)
…iero-ledger#4921) Signed-off-by: Bartosz Solka <bartosz.solka@blockydevs.com>
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #4936 +/- ##
===========================================
- Coverage 95.94% 70.34% -25.60%
===========================================
Files 143 143
Lines 23701 23839 +138
Branches 1877 632 -1245
===========================================
- Hits 22740 16770 -5970
- Misses 937 7052 +6115
+ Partials 24 17 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 85 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…ion-cumulative-gas,-not-block-total
2 participants
Description
This PR fixes an issue where cumulativeGasUsed in transaction receipts was incorrectly set to the block’s total gas used (block_gas_used) for every transaction in the block.
It updates the receipt generation logic so that cumulativeGasUsed reflects the cumulative gas consumed in the block up to and including each transaction, matching the Ethereum Yellow Paper semantics.
This change has been implemented for both eth_getBlockReceipts and eth_getTransactionReceipt.
It also updates the receipts root hash computation (and its expected test value) to use the corrected cumulative gas and adds targeted tests to validate multi‑transaction block behavior and OpenRPC contract for these endpoints.
Related issue(s)
Fixes #4921
Testing Guide
Changes from original design (optional)
N/A
Additional work needed (optional)
N/A
Checklist